-
Notifications
You must be signed in to change notification settings - Fork 12.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve cleaning of the bottom of the backtrace #41815
Conversation
r? @brson (rust_highfive has picked a reviewer for you, use r? to override) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Could a test be added for this as well?
src/libstd/sys_common/backtrace.rs
Outdated
@@ -122,7 +158,7 @@ pub fn log_enabled() -> Option<PrintFormat> { | |||
} | |||
|
|||
let val = match env::var_os("RUST_BACKTRACE") { | |||
Some(x) => if &x == "0" { | |||
Some(x) => if &x == "0" || &x == "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we could hold off on changing the interpretation of RUST_BACKTRACE
for now? I'm wary to add more and more cases for positive/negative intepretation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
let _ = resolve_symname(*frame, |symname| { | ||
if let Some(mangled_symbol_name) = symname { | ||
// Use grep to find the concerned functions | ||
if mangled_symbol_name.contains("__rust_begin_short_backtrace") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should have a deterministic symbol name, right? Would it be possible to use ==
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, we switched to a polymorphic function so we can't really do that https://github.com/Yamakaky/rust/blob/e995a17b5a7d31641b85fcf414a52c5330fff186/src/libstd/sys_common/backtrace.rs#L133
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes right, that's a good point.
Ok looks good to me! Want to squash and I'll r+? |
Yeay ! |
When `RUST_BACKTRACE=1`, remove all frames after `__rust_maybe_catch_panic`. Tested on `main`, threads, tests and benches. Cleaning of the top of the stacktrace is let to a future PR. Fixes rust-lang#40201 See rust-lang#41815
7b41ca0
to
ca8b754
Compare
(not really proud of the commit's title ^^) |
@bors: r+ |
📌 Commit ca8b754 has been approved by |
…r=alexcrichton Improve cleaning of the bottom of the backtrace Following rust-lang#40264. It only cleans the bottom of the trace (after the main). It handles correctly the normal main, tests, benchmarks and threads. I kept `skipped_before` since it will be used later for the cleaning of the top.
Improve cleaning of the bottom of the backtrace Following #40264. It only cleans the bottom of the trace (after the main). It handles correctly the normal main, tests, benchmarks and threads. I kept `skipped_before` since it will be used later for the cleaning of the top.
☀️ Test successful - status-appveyor, status-travis |
Following #40264. It only cleans the bottom of the trace (after the main). It handles correctly the normal main, tests, benchmarks and threads.
I kept
skipped_before
since it will be used later for the cleaning of the top.